Fix an update leaving Maestro unable to replace maestro-server - #329
Merged
Conversation
`deployment_version()` folds in the app version, so every update invalidates the cached server binary and `download_server_binary` re-downloads it. It finished with a bare rename onto the cached path, and Windows will not let the image of a running process be overwritten — so a surviving maestro-server.exe made that fail with ERROR_ACCESS_DENIED. Nothing retried it and the stale binary stayed in place, so the updated app could not start a single local ACP session, and relaunching only repeated the failure. Renaming a running image *is* permitted, so `install_binary_at` moves the old binary aside to `<name>.old-<nanos>` and lets the new one take its name, the same trick `install_local_link` already used for ~/.local/bin. Deleting the aside copy only works once its process exits, so `sweep_stale_binaries` reclaims it on a later run. A server survived in the first place because quitting never stopped one: `kill_on_drop(true)` fires when the `Child` is dropped inside the runtime, and `handle.exit(0)` dropped neither ACP map. Clearing both on close is what `release_active_project_lock` already does when the user leaves a project — its comment claimed to cover quitting too, but nothing called it that way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After updating on Windows, Maestro could land in a state where no local session would start — reported as "the update becomes unusable because the replacement of the maestro-server failed, the service is still running while trying to replace the file".
What was happening
deployment_version()is<app version>-protocol-<n>, so every update invalidates the cached server binary andensure_cached_binaryre-downloads it.download_server_binaryfinished with a barerename(tmp, dest)onto%APPDATA%\com.maestro.app\bin\maestro-server-windows-x86_64.exe.Windows will not let the image of a running process be overwritten. Any surviving
maestro-server.exemade that rename fail withERROR_ACCESS_DENIED, and the error was fatal with no retry —download_server_binary→ensure_cached_binary→ensure_local_server→open_local_transport. The stale binary stayed in place, so relaunching just repeated the failure, and the.download-tmpleaked on every attempt.A server survived quitting because nothing stopped it.
kill_on_drop(true)only fires when theChildis dropped inside the runtime, andhandle.exit(0)dropped neither ACP map.release_active_project_lockdoes clearconnection_servers— its comment even claims that covers "leaving the project or quitting" — but the quit path never called it that way. An installer-forced kill or a crash leaves the same orphan with no chance to exit at all.The fix
Make the swap survive a locked destination. Renaming a running image is permitted on Windows even though overwriting and deleting are not, so
install_binary_atmoves the old binary aside to<name>.old-<nanos>and lets the new one take its name — the same trickinstall_local_linkalready used for~/.local/bina hundred lines earlier. Deleting the aside copy only succeeds once its process exits, sosweep_stale_binariesreclaims it on a later run. Failure paths restore the old binary rather than leaving nothing at the path, and clean up the partial download.Stop orphaning servers on quit.
stop_connection_serversclearsacp.sessionsandacp.connection_serversin theCloseRequestedhandler, sokill_on_dropfires.A graceful stdin-EOF shutdown was considered and does not work here: the shared reader task holds its own
writer_txclone until it sees EOF, so the pipe never closes and the wait would always time out. Clearing the maps is what the leave-a-project path has always done.Testing
cargo test -p maestro --lib— 402 passed.cargo clippy --workspace --all-targets -- -D warningsclean.Three new tests in
deploy.rs. The Windows one reproduces the real failure rather than approximating it: it copiesping.exeinto place as the destination and spawns it so a live process holds that image, asserts the plain rename fails so the test proves something, then assertsinstall_binary_atsucceeds anyway, leaves exactly one.old-sibling, and that the sweep reclaims it once the holder is killed.Not yet verified by a manual GUI run that no
maestro-server.exesurvives a normal quit; that half is verified by inspection against therelease_active_project_lockpath it mirrors.Release Notes:
🤖 Generated with Claude Code